[wip] Port to Dune #588
[wip] Port to Dune #588
Conversation
|
25ec72a switches over to using |
Hi! I just want to notice that trouble with default ubuntu ld behavior described in works for ubuntu and fails for mac (removing |
|
I've worked around the ocaml/dune#1730 limitation via a configure pass in 78cad0b, which is working well for test-enums (d72e533) for example |
|
All tests now ported; time to hook in CI |
|
I'm working on rebasing this to a set of clean commits. @talex5 is helping with deploying a CI to run through the foreign architectures without needing qemu. We'll try that on this branch as a test. |
|
The latest patchset on this branch improves by:
|
src/ctypes-foreign-base/dl.ml.win
Outdated
| let ls = String.lowercase_ascii s in | ||
| let s' = | ||
| if Filename.check_suffix ls ".so" || | ||
| Filename.check_suffix ls ".dylib" |
There was a problem hiding this comment.
Using lowercase_ascii breaks 4.02.3 compatibility. The fix is easy:
| let ls = String.lowercase_ascii s in | |
| let s' = | |
| if Filename.check_suffix ls ".so" || | |
| Filename.check_suffix ls ".dylib" | |
| let s' = | |
| if Filename.check_suffix s ".so" || | |
| Filename.check_suffix s ".dylib" |
Filename.check_suffix has done the correct thing since the dawn of time (ocaml/ocaml@4ef227d), even though it wasn't documented until the year before last (ocaml/ocaml@a7a76fd)
|
It looks like this is using |
|
We could simply drop the unthreaded variant. We could do this in stages: merge the existing port, and subsequently remove it (and the use of variants). We could also propose specific support in dune for supporting unthreaded libraries, since they’re a bit special in terms of compiler options. |
|
Merged against #651 and activated ocaml-ci on my fork. I'm seeing some failures on arm32 I need to chase down: (I need to check if this also reproduces on the mainline ctypes, or if it's a bug specific to the build rules in this PR) And oddly, a bug with a multiply defined symbols in Fedora 32 worked around by 96700a9. This doesn't happen in the mainline branch, so it might be some over-eager linkage in the test_functions/clib area in this PR. |
|
To broaden that, all the Lwt-based tests seem to fail in the PR at the moment, but only on Linux but not macOS. |
|
There's another build change just gone into |
|
The This breaks at least luv. |
|
Thank you for for continuing to work on this, @emillon. I like the goal of avoiding upper-bounds in opam-repository. I'd also keep the ability of installing |
|
Some updates. I made the following changes:
|
|
And the revdeps results are pretty satisfactory, but several packages have missing dependencies on |
|
Regarding tests on windows. The crux of the problem is that there's no way at the moment with Dune to attach tests to a package and restrict them to a particular platform (ocaml/dune#6938). To improve this, I implemented a new feature in ocaml/dune#7899: I'm happy with this solution but we might not want wait for this feature to be released (and to bump the dune dependency to >= 3.9), just to check examples. Next step for me is to do a proper differential testing of revdeps to have a definitive list of regressions. |
|
That's an extremely useful feature in dune, also handy for eio! (/cc @talex5) You could put the examples in a separate opam package for now (ctypes-examples) which may make things easier. Also, feel free to push your branch to my fork if useful, as then it'll show up against this PR (and preserve comments/etc). |
Actually I reused the workaround in eio, which consists in using OCaml syntax to evaluate the
I don't have the rights to do that but I'm keeping the branch updated. Next: revdeps! |
That workaround breaks embedding the repository into a monorepo as a subdirectory. |
|
Thanks, I pushed my branch to yours.
Can you tell me more about that? Specifically I'm referring to this. I tried to build ctypes in a subdirectory and it succeeded, but maybe more is needed to demonstrate the issue. |
|
I ran some differential testing of 0.20.2 vs 0.20.2+dune3 (as tagged on my fork; 16db268 here). Considering the packages that failed on 0.20.2+dune3 but didn't on 0.20.2, there are:
All the fixes I've tried are fairly easy: replace The testing plan seems pretty extensive:
In terms of next steps, I'll leave that up to you but I would suggest:
Steps with (*) are where I can help. Let me know what you think. |
|
The plan sounds good to me. I can make a release once you let me know that these changes are ready to merge. |
|
To conclude the testing saga:
So it's time for me to squash this and write a very long commit message. |
|
I squashed the work with an extensive commit message and pushed. |
Motivation ========== With a port to dune, the ctypes library can be embedded in larger dune projects simply by including it in the directory tree of the bigger project. This in turn will allow MirageOS unikernels to use Ctypes seamlessly as part of embedded compilation, using the standard cross-compilation and library variants support built into Dune. The goal is to make Ctypes the default FFI interface to MirageOS, and have it work out of the box on all of the backends. All existing OS platforms should be supported as well before merge, including Windows. Porting Approach ================ This PR still installs ocamlfind libraries in the same way as the previous Makefile infrastructure, so backwards compat is preserved. However, the findlib schema has been slightly modified to separate out the foreign library from the core ctypes library. We now install: - ctypes that contains ctypes.top and ctypes.stubs as before. There are aliases to ctypes.foreign that redirect existing uses of those. - ctypes-foreign contains the foreign library. All of the configuration logic for libffi is now in src/ctypes-foreign-base, so deleting these directories will remove the ffi build logic without touching the core library. Since dune by default has stricter warnings enabled (the default --profile=dev mode), the PR currently sprinkles files with [@@@warning tags. The warnings can be fixed as well if desired, but that would muddy this PR so not done yet. Layout ====== The previous packaging would install ctypes and ctypes.stubs in the same library. Depending on the build system, it means that it's possible to specify a dependency on ctypes and use ctypes.stubs succesfully. This is not the case with Dune since it installs these in different directories. This means that it is necessary to patch these reverse dependencies, though it can be argued that they were relying on a bug. Followup work ============= A test uses OCaml syntax to skip test on windows. Once it's acceptable to use Dune 3.9, it should be updated to use build_if. Test and dynamic libraries ========================== Due to a different linking model, there are some changes in the test suite regarding dynamic library loading. Dynamic symbols in ocaml <4.06 ------------------------------ The setup for tests is that each test executable tests some properties using both "stubs" and "foreign" strategies to refer to some symbols define in `tests/clib/` (the `test_functions` library). The symbols are accessed through both strategies: - as a linked symbol (through an `external` via stubs) - through the dynamic loader (through `dlopen` via foreign) Dune links the stubs statically, so by default the symbols would not be visible to the dynamic loader. OCaml sets `-Wl,-E` in `LDFLAGS` to make these symbols visible at runtime, but until ocaml/ocaml@edbba02 (between 4.05 and 4.06), this was ignored when linking executables. So this commit adds these flags when building tests for older versions. An alternative would be to use `(link_flag)` in an `(env)` stanza but this requires dune 3. This issue does not affect the original make-based build because it assembles the test executables a bit differently: the `test_functions` library is linked dynamically to the test executable. So the symbols are already visible to the dynamic loader, even when `-Wl,-E` is not set. clib loading ------------ This change is specific to windows (but does not change the behavior on Linux) It modifies the test setup a bit so that the contents of `clib` are dynamically loaded at the beginning of each test. This ensures that later foreign calls (that use the default handle) will succeed. It was not necessary before because `clib` was linked dynamically. Now that it is linked statically, dynamic symbols do not have access to the symbols in the main executable. This is a problem on windows which does not have the concept of `-Wl,--export-dynamic`. Also, on windows the stubs DLL can not be loaded directly so we recompile the library using plain `%{cc}` instead of going through ocamlopt and flexlink. Instrumentation =============== This uses dune instrumentation for coverage The instructions are now: opam install bisect_ppx dune runtest --instrument-with bisect_ppx --force bisect-ppx-report html See <https://github.com/aantron/bisect_ppx#Dune> Depext handling =============== This port uses conf-libffi instead of hardcoding depexts in ctypes-foreign.
|
There's a draft release here: ocaml/opam-repository#24101 @emillon, is that everything you need for the next step of your plan ("add required upper bounds on opam-repository in the release PR")? |
|
Great! Yes, I should be able to push to your branch and make the necessary changes on the PR directly. This is going to take some time to run and I'm off tomorrow so I'll probably have to finish that on Monday. |
|
Here's a WIP list of revdeps and fixes:
|
|
I sent PRs to the various projects, links are above. |
|
How do you recommend handling the missing version information in the 0.21.0 release? For example, we could
|
|
Ah I should have mentioned that: I added a patch in the release PR that edits the |
|
ocaml/opam-repository#24101 hasn't been merged yet. It's not too late to respin the 0.21.0 release to add the version patch, or replace the package in that PR with a 0.21.1 (if you prefer not to slip the tag). Where possible, we prefer not to carry patches in the opam-repository and have them upstream. |
|
And announced on Discuss, so closing this PR as completed. Thank you for all the help pushing this over the finish line, @emillon and @yallop! https://discuss.ocaml.org/t/ann-ctypes-0-21-1-now-with-dune-support/12675 |
This is a followon from #574 with the commit history cleaned up for easier intermediate examination of the approach. The current tree builds the libraries and examples with a single
dune build, including all the libffi and other configuration logic. This is still a work-in-progress PR and will be rebased a few more times.Motivation
With a port to
dune, the ctypes library can be embedded in larger dune projects simply by including it in the directory tree of the bigger project. This in turn will allow MirageOS unikernels to use Ctypes seamlessly as part of embedded compilation, using the standard cross-compilation and library variants support built into Dune. The goal is to make Ctypes the default FFI interface to MirageOS, and have it work out of the box on all of the backends. All existing OS platforms should be supported as well before merge, including Windows.Porting Approach
This PR still installs ocamlfind libraries in the same way as the previous Makefile infrastructure, so backwards compat is preserved. However, the findlib schema has been slightly modified to separate out the
foreignlibrary from the core ctypes library. We now install:ctypesthat containsctypes.topandctypes.stubsas before. There are aliases toctypes.foreignthat redirect existing uses of those.ctypes-foreigncontains thebase,unthreadedandthreadedlibraries.All of the configuration logic for libffi is now in
src/ctypes-foreign-base, so deleting these directories will remove the ffi build logic without touching the core library.Since dune by default has stricter warnings enabled (the default
--profile=devmode), the PR currently sprinkles files with[@@@warningtags. The warnings can be fixed as well if desired, but that would muddy this PR so not done yet.Build Time
Some crude benchmarks on the clean build time show a modest improvement with dune:
(The dune build isn't fully optimised yet -- in particular, the C symbol probing can be sped up with a single invocation to cc instead of 20).
TODO
threadsfor foreign, see exposing the mt predicate to dune libraries ocaml/dune#1724 (fixed by Remove the threaded/unthreaded split in ctypes-foreign. #651)as-neededtest for cc still needed? It is, so re-add it (and tests with ubuntu 12.04 might trigger it)dune build @docis reasonable and that the Foreign variants are generated correctly despite the module clash (see Odoc: valid module name clashes cause issues ocaml/dune#1645)